Skip to content

Allow webhook secrets to be added for GitHub and GitLab#22476

Open
oliverguenther wants to merge 3 commits intodevfrom
fix/github-webhook-secret
Open

Allow webhook secrets to be added for GitHub and GitLab#22476
oliverguenther wants to merge 3 commits intodevfrom
fix/github-webhook-secret

Conversation

@oliverguenther
Copy link
Copy Markdown
Member

@oliverguenther oliverguenther force-pushed the fix/github-webhook-secret branch from a5b8797 to 6ec5ad1 Compare March 26, 2026 12:26
@oliverguenther oliverguenther force-pushed the fix/github-webhook-secret branch from 6ec5ad1 to 8f9f0d1 Compare March 26, 2026 12:32
@oliverguenther oliverguenther force-pushed the fix/github-webhook-secret branch from 8f9f0d1 to 27ada0a Compare March 26, 2026 12:47
@oliverguenther oliverguenther marked this pull request as ready for review March 26, 2026 12:50
@oliverguenther oliverguenther force-pushed the fix/github-webhook-secret branch from 27ada0a to 3c2acfc Compare March 26, 2026 13:35
token_header = request.env["HTTP_X_GITLAB_TOKEN"]
return false if token_header.blank?

ActiveSupport::SecurityUtils.secure_compare(secret, token_header)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow ... Interesting that they just send the shared secret... A signature makes so much more sense...

Comment on lines +58 to +62
menu.push :admin_integrations,
{ controller: "/github_integration/admin/settings", action: "show" },
if: ->(_) { User.current.admin? },
icon: :"git-compare",
caption: :label_integrations
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels wrong to have the parent menu item being added here. We also give "preference" to GitHub by making it the link from the integrations page. Especially since the gitlab plugin also relies on this being there. Maybe we can add this to the menus.rb initializer and have the 2 integrations hook into them.

Copy link
Copy Markdown
Contributor

@klaustopher klaustopher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General approve. Feel free to take or leave the advice about the menu refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants